backmerge#148
Conversation
<!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/su-its/core/pull/145" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end -->
…146) ## Summary DB ドライバを `drizzle-orm/node-postgres` (`pg`) から Supabase 推奨の `drizzle-orm/postgres-js` (`postgres`) に切り替える。 ## Why ### 1. Transaction pool mode 対応 Supabase の Transaction pool mode（pgbouncer 経由、port 6543）は prepared statement をサポートしない。`postgres-js` の `prepare: false` オプションを明示することで、プーラー経由の接続で安全にクエリを実行できる。 ### 2. SSL 証明書検証の問題 最新の `pg` (v8.x 系の後期) は `sslmode=require` を `verify-full` のエイリアスとして扱うようになり、Supabase の証明書チェーンで `SELF_SIGNED_CERT_IN_CHAIN` エラーが発生するようになった。これにより its-discord から接続できなくなる問題が発生していた。 `postgres-js` はこの挙動に影響されず、プーラー URL でそのまま接続できる。 ### 3. Supabase 公式推奨 Supabase の Drizzle 接続ドキュメントでも `postgres-js` + `prepare: false` が推奨されている。 ## Changes - `drizzle-orm/node-postgres` → `drizzle-orm/postgres-js` - 依存パッケージ: `pg` / `@types/pg` を削除し `postgres` を追加 - `src/infrastructure/drizzle/client.ts` で `postgres(connectionString, { prepare: false })` を使用 ## Test plan - [x] `npm run build` が成功すること - [x] its-discord から `getMemberByDiscordId` が正常動作することを確認 - [ ] 既存のテストが通ること <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/su-its/core/pull/146" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| export function getDb() { | ||
| return drizzle(getPool(), { schema }); | ||
| return drizzle(getClient(), { schema }); | ||
| } |
There was a problem hiding this comment.
📝 Info: getDb() creates a new drizzle instance on every call
Each call to getDb() at src/infrastructure/drizzle/client.ts:20-22 creates a new drizzle(getClient(), { schema }) wrapper, while the underlying postgres connection pool (client) is correctly cached as a singleton. This was the same pattern used with the previous pg Pool implementation. The drizzle wrapper is lightweight, so this is not a performance concern, but caching the drizzle instance (similar to how client is cached) would be a minor improvement for consistency.
Was this helpful? React with 👍 or 👎 to provide feedback.
| let client: Sql | null = null; | ||
|
|
||
| function getPool(): Pool { | ||
| if (!pool) { | ||
| function getClient(): Sql { | ||
| if (!client) { | ||
| const connectionString = process.env.DATABASE_URL; | ||
| if (!connectionString) { | ||
| throw new Error("DATABASE_URL environment variable is not set"); | ||
| } | ||
| pool = new Pool({ connectionString }); | ||
| // Supabase の Transaction pool mode は prepared statement をサポートしないため無効化 | ||
| client = postgres(connectionString, { prepare: false }); | ||
| } | ||
| return pool; | ||
| return client; | ||
| } |
There was a problem hiding this comment.
📝 Info: No connection cleanup / graceful shutdown handling
The postgres client exposes a .end() method for graceful shutdown, but neither the old pg.Pool code nor the new postgres code includes any cleanup mechanism. For a library package (this is @shizuoka-its/core), this is likely intentional — the consuming application is responsible for process lifecycle management. However, in long-running server environments, orphaned connections could be an issue if the module is loaded/unloaded dynamically.
Was this helpful? React with 👍 or 👎 to provide feedback.
| // Supabase の Transaction pool mode は prepared statement をサポートしないため無効化 | ||
| client = postgres(connectionString, { prepare: false }); |
There was a problem hiding this comment.
📝 Info: prepare: false disables prepared statements globally
Setting prepare: false at src/infrastructure/drizzle/client.ts:15 is necessary for Supabase Transaction pool mode (as noted in the comment), but it disables prepared statements for all queries. This has a minor performance implication — the database server will parse and plan every query from scratch rather than reusing cached plans. This is a reasonable trade-off for Supabase compatibility, but worth noting if this library is also used against non-pooled PostgreSQL instances where prepared statements would be beneficial.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Updates the Drizzle PostgreSQL client implementation and publishes a stable release version.
Changes:
- Switch Drizzle adapter from
drizzle-orm/node-postgres+pgtodrizzle-orm/postgres-js+postgres. - Disable prepared statements for compatibility with Supabase Transaction Pool mode.
- Bump package version to
3.0.1and update dependencies/lockfile accordingly.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/infrastructure/drizzle/client.ts | Migrates DB client initialization to postgres and drizzle-orm/postgres-js. |
| package.json | Updates version and swaps pg for postgres. |
| package-lock.json | Lockfile updates reflecting dependency changes and version bump. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,22 +1,24 @@ | |||
| import { drizzle } from "drizzle-orm/node-postgres"; | |||
| import { Pool } from "pg"; | |||
| /// <reference types="node" /> | |||
There was a problem hiding this comment.
The /// <reference types="node" /> directive is the only occurrence in the repo and is likely redundant given @types/node is already a devDependency and tsconfig.json doesn’t restrict types. Keeping this can cause the generated .d.ts for this module to include an implicit dependency on Node types for downstream consumers. Prefer removing the directive (or, if Node types truly need to be forced project-wide, configure it via tsconfig.json instead).
| /// <reference types="node" /> |
Uh oh!
There was an error while loading. Please reload this page.